Improve compiler single-file run syntax#9171
Conversation
|
|
||
| it "treats all arguments post-filename as program arguments" do | ||
| with_tempfile "args_test" do |path| | ||
| `bin/crystal '#{compiler_datapath}/args_test.cr' -Dother_flag -- bar '#{path}'` |
There was a problem hiding this comment.
I think referencing bin/crystal explicitly should be avoided if at all possible.
This would immediately fail on Windows, of course.
Maybe there should be a helper like this
Line 289 in 127ea04
Also this is really bad in terms of shell quoting. Please use Process.run with explicit args.
Also needs File.join.
| end | ||
|
|
||
| if single_file | ||
| opts.before_each do |arg| |
There was a problem hiding this comment.
Does it distinguish flags that have a separate value?
E.g. would it understand --exclude-warnings foo.cr bar.cr?
Maybe worth a test.
|
Does this mean executing like this doesn't work anymore? No big deal although I use it many times when I run something, press "up" to recall and add parameters. Now they have to be added before the file name. But this is actually a breaking change for any script out there. |
|
Resolves #6291 |
Yes, this changed. Ideally
|
|
In my mind What I do with my scripts: I compile them once, in release, and put them in Of course others don't need to do this... but I guess people are using Crystal as a scripting language, which boots fast, which is not. |
|
It is ok for few cases. For example, CI scripts would need to be recompiled each time anyway. We don't always need compile time speed matching scripting languages (some compiles to bytecodes before running). |
compiling to bytecode is miles faster than type checking By the way, I use flags at the end all of the time. It's just so convenient to add or remove flags from the end than to go right after the Can we not introduce a |
|
If you want flags at the end, you can use |
|
Just for reference: There's also https://github.com/Val/crun for shebang integration. Also does some cool things like caching the compiled binary IIUC. I'm not sure whether this change is worth it. I can see the general use case for shebang, but is it really so useful to pass arguments? I use |
|
Oh.. I wasn't aware of |
|
I retract my comment above. If it's useful for some then I see no problem. That said, for example in |
|
Most of the time, for a little script, you don't need compiler arguments. If you do need them when running from the commandline, then you can just add I think this is the best semantics, and allowing you to do |
|
I was always confused with unexpected semantics with
BTW I was never a fan of |
Now the compiler passes all arguments after the first filename to the program.
|
Just rebased this and fixed the spec failure. Would be neat if this could be in |
|
Could we get this in for 0.35.0? |
|
I'm ok with this change, but why is the behaviour different from |
I like this, it might help clarify the semantics! But, is this required for merging this PR? Adding script caching is something I certainly considered too. I think the fact that |
That's what I was wondering. Because this PR might promote using just |
|
It's pretty clear that it'd be Using |
|
Do we count compiler commandline behaviour as part of the |
|
@RX14 that is a good question as a user of crystal I would hope the CLI interface was consistent between versions. |
|
@RX14 I think we should make everything as a guarantee after So, to clarify, after this PR is merged:
I would agree with that behaviour. Could you please open an issue to create |
|
Maybe we need a spec to ensure the single file mode allows compiler flags before the file, right? I understand that that flag won't be able to be used in the shebang. Should we discuss how the compiler flags can be passed in that scenario? Other than that, I agree with what @waj said. |
|
Maybe this is not known, but you can do: that will consider both files for compilation and produce a single executable. Does this PR change that behavior? I'm not saying that it's a good idea to keep that (I only use that when wanting to run multiple specs, but doing |
|
I think having |
|
The current compiler syntax is a mess. Adding
I agree with all of you that this area of the compiler has serious flaws and needs review, but it needs a lot more work than can comfortably fit in this PR, and so I'd like to merge an improvement... |
|
Here is the current logic the compiler uses to determine whether to half-emulate crystal/src/compiler/crystal/command.cr Line 116 in c2113aa I'm really not sure how to add options to this, since |
I don't think we should leave this inconsistency right before 1.0. Is it easy to fix? Everything after the filename should be an argument to the program, and everything before the filename should be an argument to the compiler (according to what seems to be desired). |
|
No @asterite , because Ok @RX14, we can leave compiler flags for later in script if it is an implementation detail. It will be another feature to the command and can be added later. I think that adding |
|
Just as a summary (@waj made one, but I think this is more complete):
|
Note: adding this isn't a breaking change after 1.0, luckilly |
Though it's not clear how it would be done (or even if it's needed at all). |
Now the compiler passes all arguments after the first filename to the program.
This allows crystal to be used as
#!/usr/bin/env crystaland have arguments work correctly.